-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(valid-describe): add suggestion to disable for dynamic tests #253
docs(valid-describe): add suggestion to disable for dynamic tests #253
Conversation
Think CI just needs to be restarted |
@chrisdothtml Thanks for taking time to fix this! |
Ah, good point; I was just covering the cases mentioned in #203
Even further than that, In that light, perhaps the best way to resolve this is to just remove the EDIT: Although, the same points could be made for the |
If we could extract some type info that it's a string it might make sense, but at that point it's not really a lint rule's responsibility. Thoughts on making docs and the error say something like "must be string literal" or something, and recommending disabling the rule if you have dynamic names (like you mention)? |
9b2712e
to
519a853
Compare
Yeah I think that's the best solution. Updated my PR to just a docs update |
00311e5
to
1902be0
Compare
@@ -45,7 +45,8 @@ ruleTester.run('valid-describe', rule, { | |||
code: 'describe(() => {})', | |||
errors: [ | |||
{ | |||
message: 'First argument must be name', | |||
message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the code - this should keep the old message. Can we differentiate between missing first arg/wrong type and a variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I follow. Could you explain what exactly you're looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point is that the new message suggests that the mistake was to pass a function instead of a string, when the mistake is really that the name is just missing before the function. The old message was neutral in this regard.
And I guess @SimenB's suggestion (second sentence) was to print something like "First argument must be name" in this case because we can infer that the user probably just forgot the name in before the function, and only if both args are there but the first is not a string literal print the new one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant. Thanks! 😀
Seeing as how this hasn't been merged in yet, I'm just gonna chime in with a different opinion: the If people want typechecking, they can use If the community insists on this being a rule for the linter, then the message that it expresses should make it really clear that this might not actually be an issue. E.g. instead of
it could be
^if something like that is needed, then... it's a code smell and probably shouldn't be something the linter handles. My $.02. EDIT: the "catching |
What @thisissami suggested, or provide a config object that allows ignoring the name check |
Closing as I don't plan on finishing this PR, so anyone can free to pick up the change. Doesn't seem like there's a solid verdict on how this should be handled though |
Fixes #203